Skip to content

Address binskim flag for compiler in driver build #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 436 commits into
base: npu/release/19.x
Choose a base branch
from

Conversation

ShaojieZhuIntel
Copy link

@ShaojieZhuIntel ShaojieZhuIntel commented Feb 11, 2025

Summary

Since the compiler in the driver build cannot pass the Binskim scan and shows [Explicitly disabled warnings: 4146;4244;4267], it is necessary to remove the suppression of these three warnings. LLVM code like:

-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'

     -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
     -wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
     -wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'

-wd4146 means disable the /w4146 warning
-wd4244 means disable the /w4244 warning
-wd4267 means disable the /w4267 warning

So we need enable these three warnings to pass the Binskim

Since it is not possible to directly enable specific warnings for the LLVM repo within the compiler repo, we use an option here to control whether the specified warnings are suppressed.

JIRA ticket

Related PR in NPU Compiler and/or OpenVINO repository with sub-module update

Other related tickets

List tickets for additional work, eg, something was found during review but you agreed to address it in another Jira

  • E-xxxxx

atrosinenko and others added 30 commits February 26, 2024 17:38
Add AEK_PAUTH to ARMV8_3A in TargetParser and let it propagate to
ARMV8R, as it aligns with GCC defaults.

After adding AEK_PAUTH, several tests from TargetParserTest.cpp crashed
when trying to format an error message, thus update a format string in
AssertSameExtensionFlags to account for bitmask being pre-formatted as
std::string.

The CHECK-PAUTH* lines in aarch64-target-features.c are updated to
account for the fact that FEAT_PAUTH support and pac-ret can be enabled
independently and all four combinations are possible.

(cherry picked from commit a52eea6)
The Ampere1B is Ampere's third-generation core implementing a
superscalar, out-of-order microarchitecture with nested virtualization,
speculative side-channel mitigation and architectural support for
defense against ROP/JOP style software attacks.

Ampere1B is an ARMv8.7+ implementation, adding support for the FEAT
WFxT, FEAT CSSC, FEAT PAN3 and FEAT AFP extensions. It also includes all
features of the second-generation Ampere1A, such as the Memory Tagging
Extension and SM3/SM4 cryptography instructions.

(cherry picked from commit fbba818)
The Ampere1B core is enabled with a new scheduling/pipeline model, as it
provides significant updates over the Ampere1 core; it reduces latencies
on many instructions, has some micro-ops reassigned between the XY and X
units, and provides modelling for the instructions added since Ampere1
and Ampere1A.

As this is the first model implementing the CSSC instructions, we update
the UnsupportedFeatures on all other models (that have CompleteModel
set).

Testcases are added under llvm-mca: these showed the FullFP16 feature
missing, so we are adding it in as part of this commit.

This *adds tests and additional fixes* compared to the reverted #81338.

(cherry picked from commit dd1897c)
One of the whitespace fixes didn't get added to the commit introducing
the Ampere1B model.
Clean it up.

(cherry picked from commit 3369e34)
…… (#77291)

…n MIPS

Modify:
Add a global variable 'CurForbiddenSlotAttr' to save current
instruction's forbidden slot and whether set reorder. This is the
judgment condition for whether to add nop. We would add a couple of
'.set noreorder' and '.set reorder' to wrap the current instruction and
the next instruction.
Then we can get previous instruction`s forbidden slot attribute and
whether set reorder by 'CurForbiddenSlotAttr'.
If previous instruction has forbidden slot and .set reorder is active
and current instruction is CTI. Then emit a NOP after it.

Fix llvm/llvm-project#61045.

Because https://reviews.llvm.org/D158589 was 'Needs Review' state, not
ending, so we commit pull request again.

(cherry picked from commit 96abee5)
This include 2 fixes:
        1. Disallow 'f' for softfloat.
        2. Allow 'r' for softfloat.

Currently, 'f' is accpeted by clang, then LLVM meets an internal error.

'r' is rejected by LLVM by: couldn't allocate input reg for constraint
'r'.

Fixes: #64241, #63632

---------

Co-authored-by: Fangrui Song <[email protected]>
(cherry picked from commit c88beb4)
The modules used is-standard-library and is-std-library. The latter is
the name used in the SG15 proposal,

Fixes: llvm/llvm-project#82879
(cherry picked from commit b50bcc7)
In gas, .cpsetup may expand to one of two code sequences (one is related to `__gnu_local_gp`), depending on -mno-shared and -msym32.
Since Clang doesn't support -mno-shared or -msym32, .cpsetup expands to one code sequence.
The N32 condition incorrectly leads to the incorrect `__gnu_local_gp` code sequence.

```
00000000 <t1>:
   0:   ffbc0008        sd      gp,8(sp)
   4:   3c1c0000        lui     gp,0x0
                        4: R_MIPS_HI16  __gnu_local_gp
   8:   279c0000        addiu   gp,gp,0
                        8: R_MIPS_LO16  __gnu_local_gp
```

Fixes: #52785
(cherry picked from commit 860b6ed)
A symbol with an `N_ALT_ENTRY` attribute may be defined in the middle of
a subsection, so it is reasonable to opt them out of the
`.cfi_{start,end}proc` nesting check.

Fixes: llvm/llvm-project#82261
(cherry picked from commit 5b91647)
… (#83159)

In addition to being rather hard to follow, there isn't a good reason
why FatLTO shouldn't just share the same code for setting module flags
for (Thin)LTO. This patch simplifies the logic and makes sure we use set
these flags in a consistent way, independent of FatLTO.

Additionally, we now test that output in the .llvm.lto section actually
matches the output from Full and Thin LTO compilation.

(cherry picked from commit 7d8b50a)
DSE uses BatchAA, which caches queries using pairs of MemoryLocations.
At the moment, DSE may remove instructions that are used as pointers in
cached MemoryLocations. If a new instruction used by a new MemoryLoation
and this instruction gets allocated at the same address as a previosuly
cached and then removed instruction, we may access an incorrect entry in
the cache.

To avoid this delay removing all instructions except MemoryDefs until
the end of DSE. This should avoid removing any values used in BatchAA's
cache.

Test case by @vporpo from
llvm/llvm-project#83181.
(Test not precommitted because the results are non-determinstic - memset
only sometimes gets removed)

PR: llvm/llvm-project#83411
(cherry picked from commit 10f5e98)
glibc 2.39 added `nonnull` attribute to most libio functions accepting a
`FILE*` parameter, including fprintf[1]. The -fsanitize=undefined mode
checks the argument to fprintf and has extra counters, not expected by
two tests. Specify -fno-sanitize=nonnull-attribute to make the two tests
pass.

Fix #82883

[1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=64b1a44183a3094672ed304532bedb9acc707554

Pull Request: llvm/llvm-project#84231

(cherry picked from commit c3acbf6)
When replacing with a non-constant, it's possible that the result of the
simplification is actually more complicated than the original, and may
result in an infinite combine loop.

Mitigate the issue by requiring that either the replacement or
simplification result is constant, which should ensure that it's
simpler. While this check is crude, it does not appear to cause
optimization regressions in real-world code in practice.

Fixes llvm/llvm-project#83127.

(cherry picked from commit 9f45c5e)
… (#84540)

MSVC does not define __BYTE_ORDER__ making the check for BigEndian
erroneously evaluate to true and breaking the struct definitions in MSVC
compiled builds correspondingly. The fix adds an additional check for
whether __BYTE_ORDER__ is defined by the compiler to fix these.

---------

Co-authored-by: Vadim Paretsky <[email protected]>
(cherry picked from commit 110141b)
…e (#83990)

We should generate the `MCInstPredicate` twice, one with `FirstMI`
and another with `SecondMI`.

(cherry picked from commit de1f338)
…apArg (#83656)

This patch aims to solve Firefox issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1882301

Similar to 616289e. Currently LoongArch uses an ll.[wd]/sc.[wd]
loop for ATOMIC_CMP_XCHG. Because the comparison in the loop is
full-width (i.e. the `bne` instruction), we must sign extend the input
comparsion argument.

Note that LoongArch ISA manual V1.1 has introduced compare-and-swap
instructions. We would change the implementation (return `ANY_EXTEND`)
when we support them.

(cherry picked from commit 5f058aa)
ARM64EC varargs calls expect that x4 = sp at entry, special handling is
needed to ensure this with tail calls since they occur after the
epilogue and the x4 write happens before.

I tried going through AArch64MachineFrameLowering for this, hoping to
avoid creating the dummy object but this was the best I could do since
the stack info that uses isn't populated at this stage,
CreateFixedObject also explicitly forbids 0 sized objects.
…s (#80595)

ISel handles filling in x4/x5 when calling variadic functions as they
don't correspond to the 5th/6th X64 arguments but rather to the end of
the shadow space on the stack and the size in bytes of all stack
parameters (ignored and written as 0 for calls from entry thunks).

Will PR a follow up with ISel handling after this is merged.
…ls (#80994)

When in an entry thunk the x64 SP is passed in x4 but this cannot be
directly passed through since x64 varargs calls have a 32 byte shadow
store at SP followed by the in-stack parameters. ARM64EC varargs calls
on the other hand expect x4 to point to the first in-stack parameter.
This re-lands cc0065a in a way that
keeps existing targets working.

---------

Original commit message:
#68132 ended up removing
__multc3 & __divtc3 from compiler-rt library builds that have
QUAD_PRECISION but not TF_MODE due to missing int128 support.
I added support for QUAD_PRECISION to use the native hex float long double representation.

---------

Co-authored-by: Sean Perry <[email protected]>
(cherry picked from commit 99c457d)
g++ -flto has a diagnostic `-Wodr` about mismatched redeclarations,
which even apply to `enum`.

Fix #83529

Reviewers: thesamesam

Reviewed By: thesamesam

Pull Request: llvm/llvm-project#83604

(cherry picked from commit 4a3f7e7)
…(#83384)

When MVT is not a vector type, TCK_CodeSize should return an invalid
cost. This patch adds a check in the beginning to make sure all cost
kinds return invalid costs consistently.

Before this patch, TCK_CodeSize returns a valid cost on scalar MVT but
other cost kinds doesn't.

This fixes the issue #83294 where a loop contains vector instructions
and MVT is scalar after type legalization when the vector extension is
not enabled,

(cherry picked from commit fb67dce)
When analysing whether we should handle a binary expression as an
overloaded operator call or a builtin operator, we were calling
`checkPlaceholderForOverload()`, which takes care of any placeholders
that are not overload sets—which would usually make sense since those
need to be handled as part of overload resolution.

Unfortunately, we were also doing that for `.*`, which is not
overloadable, and then proceeding to create a builtin operator anyway,
which would crash if the RHS happened to be an unresolved overload set
(due hitting an assertion in `CreateBuiltinBinOp()`—specifically, in one
of its callees—in the `.*` case that makes sure its arguments aren’t
placeholders).

This pr instead makes it so we check for *all* placeholders early if the
operator is `.*`.

It’s worth noting that,
1. In the `.*` case, we now additionally also check for *any*
placeholders (not just non-overload-sets) in the LHS; this shouldn’t
make a difference, however—at least I couldn’t think of a way to trigger
the assertion with an overload set as the LHS of `.*`; it is worth
noting that the assertion in question would also complain if the LHS
happened to be of placeholder type, though.
2. There is another case in which we also don’t perform overload
resolution—namely `=` if the LHS is not of class or enumeration type
after handling non-overload-set placeholders—as in the `.*` case, but
similarly to 1., I first couldn’t think of a way of getting this case to
crash, and secondly, `CreateBuiltinBinOp()` doesn’t seem to care about
placeholders in the LHS or RHS in the `=` case (from what I can tell,
it, or rather one of its callees, only checks that the LHS is not a
pseudo-object type, but those will have already been handled by the call
to `checkPlaceholderForOverload()` by the time we get to this function),
so I don’t think this case suffers from the same problem.

This fixes #53815.

---------

Co-authored-by: Aaron Ballman <[email protected]>
…es (#81133)

For a label difference like `.uleb128 A-B`, MC generates a pair of
R_LARCH_{ADD,SUB}_ULEB128 if A-B cannot be folded as a constant. GNU
assembler generates a pair of relocations in more cases (when A or B is
in a code section with linker relaxation). It is similar to RISCV.

R_LARCH_{ADD,SUB}_ULEB128 relocations are created by Clang and GCC in
`.gcc_except_table` and other debug sections with linker relaxation
enabled. On LoongArch, first read the buf and count the available space.
Then add or sub the value. Finally truncate the expected value and fill
it into the available space.

(cherry picked from commit eaa9ef6)
@hrotuna hrotuna requested a review from nikita-kud February 13, 2025 11:40
@AlexandruLorinti
Copy link

I confirm there are no errors regarding suppressed warnings in BinSkim scan

@@ -767,6 +767,15 @@ if (MSVC)
foreach(flag ${msvc_warning_flags})
append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endforeach(flag)

if (BUILD_COMPILER_FOR_DRIVER)
string(REPLACE "-wd4146" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I'm not quite familiar with LLVM CMake code involved here, but I'd consider updating global CMAKE variables, especially CMAKE_C/CXX_* not the best practice, reason is there's a chance it'd affect other targets you didn't intended to change
I saw that case in our project already, when OpenVINO did this and then propagated not always desired compilation options to our targets

If you stick with modifying these variables for some reason, could you please clarify why and confirm no unintended target is changed (presumably nothing outside of llvm-project folder)?

Alternative would be just working with target properties, you can specify compilation options per target such it's not intrusive

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to modify and test based on your suggestion.

…l#96)

Bumps the github-actions group with 8 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [step-security/harden-runner](https://github.com/step-security/harden-runner) | `2.10.1` | `2.10.4` |
| [actions/checkout](https://github.com/actions/checkout) | `4.1.1` | `4.2.2` |
| [actions/dependency-review-action](https://github.com/actions/dependency-review-action) | `4.4.0` | `4.5.0` |
| [tj-actions/changed-files](https://github.com/tj-actions/changed-files) | `45.0.3` | `45.0.6` |
| [aminya/setup-cpp](https://github.com/aminya/setup-cpp) | `0.44.0` | `0.46.0` |
| [actions/setup-python](https://github.com/actions/setup-python) | `5.3.0` | `5.4.0` |
| [ossf/scorecard-action](https://github.com/ossf/scorecard-action) | `2.3.1` | `2.4.0` |
| [actions/upload-artifact](https://github.com/actions/upload-artifact) | `97a0fba1372883ab732affbe8f94b823f91727db` | `c24449f33cd45d4826c6702db7e49f7cdb9b551d` |



Updates `step-security/harden-runner` from 2.10.1 to 2.10.4
- [Release notes](https://github.com/step-security/harden-runner/releases)
- [Commits](step-security/harden-runner@91182cc...cb605e5)

Updates `actions/checkout` from 4.1.1 to 4.2.2
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v4.1.1...11bd719)

Updates `actions/dependency-review-action` from 4.4.0 to 4.5.0
- [Release notes](https://github.com/actions/dependency-review-action/releases)
- [Commits](actions/dependency-review-action@4081bf9...3b139cf)

Updates `tj-actions/changed-files` from 45.0.3 to 45.0.6
- [Release notes](https://github.com/tj-actions/changed-files/releases)
- [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
- [Commits](tj-actions/changed-files@c3a1bb2...d6e91a2)

Updates `aminya/setup-cpp` from 0.44.0 to 0.46.0
- [Release notes](https://github.com/aminya/setup-cpp/releases)
- [Commits](aminya/setup-cpp@d485b24...12e62a1)

Updates `actions/setup-python` from 5.3.0 to 5.4.0
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@0b93645...4237552)

Updates `ossf/scorecard-action` from 2.3.1 to 2.4.0
- [Release notes](https://github.com/ossf/scorecard-action/releases)
- [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md)
- [Commits](ossf/scorecard-action@0864cf1...62b2cac)

Updates `actions/upload-artifact` from 97a0fba1372883ab732affbe8f94b823f91727db to c24449f33cd45d4826c6702db7e49f7cdb9b551d
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@97a0fba...c24449f)

---
updated-dependencies:
- dependency-name: step-security/harden-runner
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: actions/dependency-review-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: tj-actions/changed-files
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: aminya/setup-cpp
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: ossf/scorecard-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Comment on lines 295 to 296
if(MSVC)
if(BUILD_COMPILER_FOR_DRIVER)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: if (MSVC AND BUILD_COMPILER_FOR_DRIVER)
Also, since we have to modified LLVM's codebase, I suppose we should have some kind tracker of all such changes

@nikita-kud do we have anything like this already?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thank you!

Copy link
Contributor

@nikita-kud nikita-kud Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikita-kud do we have anything like this already?

Yes, we have: E-101222
But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikita-kud do we have anything like this already?

Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikita-kud do we have anything like this already?

Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?

any method would be ok, you can keep the patch in CiD build

@ShaojieZhuIntel ShaojieZhuIntel force-pushed the address-cid-binskim-patch branch from 85abebf to cb1282d Compare February 20, 2025 11:57
Copy link

@ggladilo ggladilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment to each modification of compile options
Maybe makes sense to create a CMake function that accepts target and silences all needed warnings, then you could add explanation to just function implementation
In this case all the logic for making binskim happy is reused


if(MSVC AND BUILD_COMPILER_FOR_DRIVER)
target_compile_options(LLVMAnalysis PRIVATE
/w34146 /w34244 /w34267

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to do the same in NPU Cmake which includes LLVM, avoiding a patch.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I guess it's true, we could write CMake code/function in our project that processes pre-defined set of targets (from LLVM repo that we know are problematic to one our jobs/processes) and for each given target modifies a property of it that corresponds to compile options used to build the target accordingly
This way we get clean separation - this warnings disablement is not a problem of LLVM (since they can build their targets they probably just do not provide support for these-warnings-clean-build), but our problem since we seem to place more requirements on LLVM codebase than its authors/maintainers

Comment on lines 165 to 167
# Due to this target cannot pass the binskim scan after the compiler in the driver build,
# it is necessary to remove the suppression of these three warnings(/wd4146 /wd4244 /wd4267).
enable_msvc_warnings_on_compiler_for_driver(LLVMAnalysis /w3 4146 4244 4267)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move comments to the function implementation, no need to repeat every time you call the function
Function name is confusing, do you disable or enable warnings here? From the values looks like you disable them
Also, I'd keep function specific to your use case, instead of accepting warnings as arguments, just "hardcode" them internally and name the function something along the lines as fix_binksim or something (choose better name than mine!)
Also consider @lmielick's comment to implement solution in compiler repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you for your suggestions! Here we are enabling warnings, and I will modify the function according to your advice. Additionally, I am trying to modify only the compiler repo to make it pass binskim.

dependabot bot and others added 3 commits March 10, 2025 16:19
Bumps [cryptography](https://github.com/pyca/cryptography) from 43.0.1 to 44.0.1.
- [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst)
- [Commits](pyca/cryptography@43.0.1...44.0.1)

---
updated-dependencies:
- dependency-name: cryptography
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Security fixes ww11

* Add tar.gz jinja sha256
@ShaojieZhuIntel ShaojieZhuIntel force-pushed the address-cid-binskim-patch branch from 26236fc to 5ce36b2 Compare March 27, 2025 03:53
@ShaojieZhuIntel
Copy link
Author

Firstly, can not directly enable warnings for LLVM within the compiler repo. Secondly, I tried creating a function in the compiler repo and calling this function for each target in LLVM that fails the binskim check. This approach reduces the binskim errors, but it involves modifying many targets. Additionally, if the same errors appear in other targets in the future, further modifications to the LLVM code will be necessary. Therefore, I believe it is more appropriate to add an option in LLVM that allows controlling the enabling of relevant warnings from the compiler repo.

@ShaojieZhuIntel
Copy link
Author

Test job: https://jenkins-npu-compiler.ir.intel.com/job/pub/job/IE-Packages/job/Release/job/CiD-Windows10/15966/ passed
binskim passed
Hi @ggladilo @lmielick @nikita-kud @AlexandruLorinti Could you please help to review this change?

@andrey-golubev
Copy link
Contributor

andrey-golubev commented Mar 27, 2025

Why do we need it in LLVM? Can we instead do it at the compiler's side around add_subdirectory(LLVM)?
Then the change is local to our stack and doesn't have to be forever cherry-picked between LLVM releases.

Also, removing warnings doesn't look good to me. Are there issues in LLVM otherwise? Can we fix them instead and push that to upstream?

@lmielick
Copy link

lmielick commented Mar 27, 2025

This approach reduces the binskim errors, but it involves modifying many targets.

@ShaojieZhuIntel did you try using directory level CMAKE variables? I would presume we can append these flags into CFLAGS on directory level. Even if this is applied eagerly for all LLVM targets it should be fine:
a) there is no incentive to fix warnings on LLVM side
b) if it's contingent on BINSKIM flag we can still keep the LLVM warnings on in developer build or so

@ShaojieZhuIntel
Copy link
Author

Why do we need it in LLVM? Can we instead do it at the compiler's side around add_subdirectory(LLVM)? Then the change is local to our stack and doesn't have to be forever cherry-picked between LLVM releases.

Also, removing warnings doesn't look good to me. Are there issues in LLVM otherwise? Can we fix them instead and push that to upstream?

We are not removing warnings; we are enabling warnings because some warnings are suppressed in LLVM, which is not allowed by binskim. Like:

  AliasAnalysis.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AliasSetTracker.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AssumeBundleQueries.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AssumptionCache.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BasicAliasAnalysis.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BlockFrequencyInfo.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BlockFrequencyInfoImpl.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BranchProbabilityInfo.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]

Even if we handle this around add_subdirectory(LLVM), it will be overridden by the actions in LLVM that disable warnings.

-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'

      -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
      -wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
      -wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'

@ShaojieZhuIntel
Copy link
Author

This approach reduces the binskim errors, but it involves modifying many targets.

@ShaojieZhuIntel did you try using directory level CMAKE variables? I would presume we can append these flags into CFLAGS on directory level. Even if this is applied eagerly for all LLVM targets it should be fine: a) there is no incentive to fix warnings on LLVM side b) if it's contingent on BINSKIM flag we can still keep the LLVM warnings on in developer build or so

Yes, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective. Like:

-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'

      -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
      -wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
      -wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'

@XinWangIntel
Copy link
Contributor

@ggladilo @lmielick @andrey-golubev @AlexandruLorinti , could you help to review this again? The PR is to open suppressed warnings by remove some compiler flags (which make we can not see warnings in log). And we tried on vpux-plugin repo side, append cmake flag on that level can not work since llvm append flag after that again and invalid our new added flags.

Copy link
Contributor

@nikita-kud nikita-kud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShaojieZhuIntel I'm sorry, but I would like to ask you to wait until the LLVM19 update is complete.

@andrey-golubev
Copy link
Contributor

andrey-golubev commented Apr 9, 2025

We are not removing warnings; we are enabling warnings because some warnings are suppressed in LLVM, which is not allowed by binskim.

Who should I speak to regarding this? I want to figure out whether we care enough about not-our thirdparty to have this mess forever.

Yes, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective.

@ShaojieZhuIntel but what does this do then?

# Enable warnings
if (LLVM_ENABLE_WARNINGS)
# Put /W4 in front of all the -we flags. cl.exe doesn't care, but for
# clang-cl having /W4 after the -we flags will re-enable the warnings
# disabled by -we.
set(msvc_warning_flags "/W4 ${msvc_warning_flags}")

Do we need our custom flag at all here? Can we instead use LLVM_ENABLE_WARNINGS=ON?

Copy link
Contributor

@andrey-golubev andrey-golubev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking for now: let's figure our what already present LLVM_ENABLE_WARNINGS does

@ShaojieZhuIntel
Copy link
Author

ShaojieZhuIntel commented Apr 10, 2025

We are not removing warnings; we are enabling warnings because some warnings are suppressed in LLVM, which is not allowed by binskim.

Who should I speak to regarding this? I want to figure out whether we care enough about not-our thirdparty to have this mess forever.

Yes, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective.

@ShaojieZhuIntel but what does this do then?

# Enable warnings
if (LLVM_ENABLE_WARNINGS)
# Put /W4 in front of all the -we flags. cl.exe doesn't care, but for
# clang-cl having /W4 after the -we flags will re-enable the warnings
# disabled by -we.
set(msvc_warning_flags "/W4 ${msvc_warning_flags}")

Do we need our custom flag at all here? Can we instead use LLVM_ENABLE_WARNINGS=ON?

First, we need to address the binskim issue caused by this thirdparty because the target npu_llvm_utils in VPUX is related to this thirdparty. Binskim shows
image
Secondly, enabling LLVM_ENABLE_WARNINGS can display more warnings when building LLVM, but it won't provide more information when building VPUX, and it won't enable warnings 4146, 4244, 4267, and other suppressed warnings in ${msvc_warning_flags} . However, we can remove all -wd flags from ${msvc_warning_flags} when LLVM_ENABLE_WARNINGS is ON. This approach not only enables the warnings but also resolves the binskim scan errors. The new changes have been pushed and tested to work.

# Promoted warnings to errors.
-we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error.
)
endif(NOT CLANG_CL)
Copy link
Contributor

@andrey-golubev andrey-golubev Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add these in NPU CMake to CMAKE_CXX_FLAGS instead?
something tells me it should be possible to do. and we won't need the patch here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(now that we know that ENABLE_WARNINGS works, perhaps we could move it outside?)

@ShaojieZhuIntel ShaojieZhuIntel changed the base branch from npu/release/18.x to npu/release/19.x May 15, 2025 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.